Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix the conditions history in compliancyDetails #91

Merged
merged 2 commits into from
Jan 11, 2023
Merged

Fix the conditions history in compliancyDetails #91

merged 2 commits into from
Jan 11, 2023

Conversation

mprahl
Copy link
Member

@mprahl mprahl commented Jan 10, 2023

The status.compliancyDetails[].conditions array was always limited to just a single condition due to a bug. This sets the limit to 10 instead.

Relates:
https://issues.redhat.com/browse/ACM-2708

willkutler
willkutler previously approved these changes Jan 10, 2023
View reviewed changes
@openshift-ci openshift-ci bot added the lgtm label Jan 10, 2023
dhaiducek
dhaiducek previously approved these changes Jan 10, 2023
View reviewed changes
@mprahl mprahl dismissed stale reviews from dhaiducek and willkutler via 8544c74 January 10, 2023 20:19
@mprahl mprahl requested a review from dhaiducek January 10, 2023 20:19
@openshift-ci openshift-ci bot removed the lgtm label Jan 10, 2023
dhaiducek
dhaiducek previously approved these changes Jan 10, 2023
View reviewed changes
@openshift-ci openshift-ci bot added the lgtm label Jan 10, 2023
@openshift-ci openshift-ci bot removed the lgtm label Jan 11, 2023
@mprahl mprahl requested a review from dhaiducek January 11, 2023 13:18
@mprahl mprahl force-pushed the fix-conditions branch 2 times, most recently from 92572c7 to 2b8c969 Compare January 11, 2023 14:24
@mprahl
Fix the conditions history in compliancyDetails
88c6fd2 
The status.compliancyDetails[].conditions array was always limited to
just a single condition due to a bug. This sets the limit to 10 instead.

Relates:
https://issues.redhat.com/browse/ACM-2708

Signed-off-by: mprahl <mprahl@users.noreply.github.com>
@mprahl
Add an eventually in the CRD deletion case
3c98e22 
Signed-off-by: mprahl <mprahl@users.noreply.github.com>
@dhaiducek
Copy link
Member

I've been thinking about this, and I don't think it's a bug per se. This looks intentional since the status-sync is the thing that has the 10 history limit, so the Policy as the source of the history and the ConfigurationPolicy as only holding the current state. Though I could see the ConfigurationPolicy not having the same information as the Policy as a bug, but maybe we only need to address this in 2.7 as a result? Does the current behavior cause issues or is it just an inconsistency?

@willkutler
Copy link
Contributor

@dhaiducek it doesn't cause issues with the status of the policy at all (besides the history inconsistency with config policies) but I would still say it's a bug since we have a conditions array that isn't really being used. If we want to only hold the latest condition in the config policy and have policies be responsible for history, that would be fine but I think we'd want the status.compliancyDetails[].conditions field to be a map rather than an array of maps (this would be more of a 2.8 change). If we keep it as an array, I think it makes sense for it to hold a history, since that seems to be the intended behavior based on how we wrote the config policy CRD.

@mprahl
Copy link
Member Author

mprahl commented Jan 11, 2023

I've been thinking about this, and I don't think it's a bug per se. This looks intentional since the status-sync is the thing that has the 10 history limit, so the Policy as the source of the history and the ConfigurationPolicy as only holding the current state. Though I could see the ConfigurationPolicy not having the same information as the Policy as a bug, but maybe we only need to address this in 2.7 as a result? Does the current behavior cause issues or is it just an inconsistency?

@dhaiducek the status-sync gets its history from status events sent by the ConfigurationPolicy, so the conditions array doesn't influence that. Having a history in the ConfigurationPolicy is still desirable as it contains more information than the status stored by the status-sync, in particular it shows information per object-template entry.

@mprahl
Copy link
Member Author

mprahl commented Jan 11, 2023

Sorry about the noise on this PR @dhaiducek and @willkutler, but it's rebased on #90 and the tests are now passing.

@openshift-ci openshift-ci bot added the lgtm label Jan 11, 2023
dhaiducek
dhaiducek approved these changes Jan 11, 2023
View reviewed changes
Copy link
Member

@dhaiducek dhaiducek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanations/background! I think I was questioning whether a backport was necessary since the issue is targeted for 2.5 and 2.6, but it does seem like it'd be nice to have for debugging and consistency.

@openshift-ci
Copy link

openshift-ci bot commented Jan 11, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dhaiducek, mprahl, willkutler

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [dhaiducek,mprahl,willkutler]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit aaf185e into open-cluster-management-io:main Jan 11, 2023
@magic-mirror-bot magic-mirror-bot bot mentioned this pull request Jan 11, 2023
Merged
@mprahl mprahl deleted the fix-conditions branch January 11, 2023 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dhaiducek dhaiducek dhaiducek approved these changes

@willkutler willkutler willkutler approved these changes

@gparvin gparvin Awaiting requested review from gparvin

@JustinKuli JustinKuli Awaiting requested review from JustinKuli

Assignees

@dhaiducek dhaiducek

@willkutler willkutler

Labels
approved dco-signoff: yes lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mprahl @dhaiducek @willkutler @openshift-merge-robot